[PS5] Enable support for DTLTO in the PS5 Clang driver #158041
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: bd1976bris (bd1976bris) ChangesDTLTO support was added for most targets via the shared Unlike other drivers, we add LTO-related options unconditionally. This makes sense because the linker decides whether to perform LTO based on input file types, not the presence of Internal-Ref: TOOLCHAIN-19896 Full diff: https://github.com/llvm/llvm-project/pull/158041.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/PS4CPU.cpp b/clang/lib/Driver/ToolChains/PS4CPU.cpp
index 21e23d486f9d4..d77f09ce50fff 100644
--- a/clang/lib/Driver/ToolChains/PS4CPU.cpp
+++ b/clang/lib/Driver/ToolChains/PS4CPU.cpp
@@ -343,6 +343,18 @@ void tools::PS5cpu::Linker::ConstructJob(Compilation &C, const JobAction &JA,
// whether or not that will be the case at this point. So, unconditionally
// pass LTO options to ensure proper codegen, metadata production, etc if
// LTO indeed occurs.
+
+ if (Arg *A = Args.getLastArg(options::OPT_fthinlto_distributor_EQ)) {
+ CmdArgs.push_back(
+ Args.MakeArgString("--thinlto-distributor=" + Twine(A->getValue())));
+ CmdArgs.push_back(
+ Args.MakeArgString("--thinlto-remote-compiler=" +
+ Twine(D.getClangProgramPath())));
+
+ for (auto A : Args.getAllArgValues(options::OPT_Xthinlto_distributor_EQ))
+ CmdArgs.push_back(Args.MakeArgString("--thinlto-distributor-arg=" + A));
+ }
+
if (Args.hasFlag(options::OPT_funified_lto, options::OPT_fno_unified_lto,
true))
CmdArgs.push_back(D.getLTOMode() == LTOK_Thin ? "--lto=thin"
diff --git a/clang/test/Driver/DTLTO/ps5-dtlto.c b/clang/test/Driver/DTLTO/ps5-dtlto.c
new file mode 100644
index 0000000000000..9b70c88257a85
--- /dev/null
+++ b/clang/test/Driver/DTLTO/ps5-dtlto.c
@@ -0,0 +1,45 @@
+// REQUIRES: lld
+
+/// Check DTLTO options are forwarded to the linker.
+
+/// Check that options are forwarded as expected with --thinlto-distributor=.
+// RUN: %clang -flto=thin %s -### --target=x86_64-sie-ps5 \
+// RUN: -Xthinlto-distributor=a1 -Xthinlto-distributor=a2,a3 \
+// RUN: -fthinlto-distributor=d.exe -Werror 2>&1 | \
+// RUN: FileCheck %s --check-prefix=FORWARD
+
+// FORWARD: prospero-lld
+// FORWARD-SAME: "--thinlto-distributor=d.exe"
+// FORWARD-SAME: "--thinlto-remote-compiler={{[^"]+}}"
+// FORWARD-SAME: "--thinlto-distributor-arg=a1"
+// FORWARD-SAME: "--thinlto-distributor-arg=a2"
+// FORWARD-SAME: "--thinlto-distributor-arg=a3"
+
+/// Check that options are not added without --thinlto-distributor= and
+/// that a warning is issued for unused -Xthinlto-distributor options.
+// RUN: %clang -flto=thin %s -### --target=x86_64-sie-ps5 \
+// RUN: -Xthinlto-distributor=a1 -Xthinlto-distributor=a2,a3 2>&1 | \
+// RUN: FileCheck %s --check-prefix=NODIST --implicit-check-not=distributor \
+// RUN: --implicit-check-not=remote-compiler
+
+// NODIST: warning: argument unused during compilation: '-Xthinlto-distributor=a1'
+// NODIST: warning: argument unused during compilation: '-Xthinlto-distributor=a2,a3'
+// NODIST: prospero-lld
+
+/// Check the expected arguments are forwarded by default with only
+/// --thinlto-distributor=.
+// RUN: %clang -flto=thin %s -### --target=x86_64-sie-ps5 \
+// RUN: -fthinlto-distributor=d.exe -Werror 2>&1 | \
+// RUN: FileCheck %s --check-prefix=DEFAULT --implicit-check-not=distributor \
+// RUN: --implicit-check-not=remote-compiler
+
+// DEFAULT: prospero-lld
+// DEFAULT-SAME: "--thinlto-distributor=d.exe"
+// DEFAULT-SAME: "--thinlto-remote-compiler={{.*}}clang{{[^\"]*}}"
+
+/// Check that the arguments are forwarded unconditionally even when the
+/// compiler is not in LTO mode.
+// RUN: %clang %s -### --target=x86_64-sie-ps5 \
+// RUN: -fthinlto-distributor=d.exe -Werror 2>&1 | \
+// RUN: FileCheck %s --check-prefix=DEFAULT --implicit-check-not=distributor \
+// RUN: --implicit-check-not=remote-compiler
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
DTLTO support was added for most targets via the shared `addLTOOptions` helper. The PS5 driver does not call that helper, so it did not inherit the feature. Implement the equivalent DTLTO handling in the PS5 driver. Unlike other drivers, we add LTO-related options unconditionally. This makes sense because the linker decides whether to perform LTO based on input file types, not the presence of `-flto` on the compiler command line. Other drivers only add these options when `-flto` is specified. Internal-Ref: TOOLCHAIN-19896
bd1976bris
force-pushed
the
dtlto-in-ps5-compiler-driver
branch
from
b818192 to
5575350
Compare
There was a problem hiding this comment.
LGTM with some nits inline; I'd leave the final nod to @playstation-edd as he's done more driver work lately.
| CmdArgs.push_back(Args.MakeArgString("--thinlto-remote-compiler=" + | ||
| Twine(D.getClangProgramPath()))); | ||
|
|
||
| for (auto A : Args.getAllArgValues(options::OPT_Xthinlto_distributor_EQ)) |
There was a problem hiding this comment.
Nit, auto &A or auto *A to avoid un-necessary copies? Might not be possible with this type, I can't remember what getAllArgValues returns.
There was a problem hiding this comment.
Nice. Thanks. I have made the same change for the equivalent code in addLTOOptions.
There was a problem hiding this comment.
The change addLTOOptions is unrelated to the purpose of this PR, so I would personally prefer it's removal.
I would like to consider refactoring what we're doing here to share/align implementation with other drivers. That's probably the point at which addLTOOptions could be adjusted.
| /// Check that the arguments are forwarded unconditionally even when the | ||
| /// compiler is not in LTO mode. |
There was a problem hiding this comment.
Will "unused argument" warnings be produced, and should we check for them?
There was a problem hiding this comment.
No. Unlike most drivers we consume the arguments even if we are not in LTO mode (-flto is not specified). -Xthinlto-distributor= arguments are not consumed if '-fthinlto-distributor=' is not supplied but we check for that on line 25 of this test. These arguments are not consumed if we are not linking e.g. -c is specified, but that's in common with all the arguments that are processed here in Linker::ConstructJob.
bd1976bris
force-pushed
the
dtlto-in-ps5-compiler-driver
branch
from
3f9ae2d to
6fa31fd
Compare
…bots Not all builds name the compiler executable clang. For example, the Fuchsia buildbots use llvm as their single toolchain executable name, as they combine everything together in a busybox-style binary. This is currently causing the new ps5-dtlto.c to fail on such build bots. Update the Clang driver tests to simply check that a non-empty path is provided for the --thinlto-remote-compiler argument, rather than hardcoding the executable name. The cross-project tests will verify that the path is valid later. This is the same fix as applied earlier in llvm#148908. However, that fix left a case in the dtlto.c test that was subsequently reflected into the new ps5-dtlto.c test where it caused a failure. Why it doesn't cause a failure in the existing dtlto.c test is a mystery to me - perhaps the substring "clang" is now included in the path to the busybox-style binary, or perhaps that test was disabled for affected buildbots and then not re-enabled? It's clearly a latent issue though so I have also fixed the dtlto.c test in this patch. Should fix the buildbot failures caused by: llvm#158041.
|
The test |
|
Belated FYI: @jmagee, @kbelochapka, @romanova-ekaterina, @nga888 |
4 participants
DTLTO support was added for most targets via the shared
addLTOOptionshelper. The PS5 driver does not call that helper, so it did not inherit the feature. Implement the equivalent DTLTO handling in the PS5 driver.Unlike other drivers, we add LTO-related options unconditionally. This makes sense because the linker decides whether to perform LTO based on input file types, not the presence of
-fltoon the compiler command line. Other drivers only add these options when-fltois specified.Internal-Ref: TOOLCHAIN-19896